Summary of Changes

Hello @martin-augment, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a critical memory leak vulnerability within the Avro C library, specifically targeting scenarios involving malformed or abnormal input data. By introducing a mechanism to determine the maximum readable buffer size and a new safe read macro that ensures proper memory deallocation upon read failures, the changes significantly enhance the library's robustness and prevent resource exhaustion when processing potentially malicious or corrupted data streams.

Highlights

• Introduction of avro_max_read function: This new function, declared in avro/io.h and implemented in io.c, calculates the maximum number of bytes that can be safely read from an Avro reader, supporting both memory and file-based I/O.
• Creation of AVRO_SAFE_READ macro: A new macro in encoding.h that wraps the avro_read function. It adds a crucial safety mechanism to free allocated memory (buf) and set it to NULL if the read operation fails, thereby preventing memory leaks.
• Enhanced byte and string reading: The read_bytes and read_string functions in encoding_binary.c now utilize the AVRO_SAFE_READ macro. Additionally, read_string incorporates a new check using avro_max_read to validate that the declared string length does not exceed the available buffer size, mitigating potential buffer overflow risks.

Walkthrough

The changes add a new public API function avro_max_read() to query available bytes for reading in the Avro C library. A corresponding macro AVRO_SAFE_READ() wraps avro_read calls to handle errors by freeing allocated memory and nullifying pointers. The binary encoding functions for reading strings and bytes are updated to use this macro and perform bounds checking before reading data. Implementation and declarations are added to io.c and io.h, with the macro defined in encoding.h.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Pull Request Review: Memory Leak Fix in Avro C Library

Summary

This PR addresses a memory leak issue in the Avro C library when handling abnormal/malformed data. The fix adds bounds checking and safe cleanup mechanisms for string and bytes reading operations.

Code Quality & Best Practices

✅ Strengths

1. Good defensive programming: The addition of bounds checking prevents buffer overruns when malformed data claims a string/bytes length larger than available buffer space.

2. Consistent error handling: The new AVRO_SAFE_READ macro follows the existing pattern established by AVRO_READ, AVRO_WRITE, and AVRO_SKIP macros.

3. Clear error messages: The added error message in read_string (line 196-197 in encoding_binary.c) provides helpful diagnostic information.

⚠️ Issues to Address

1. Code Style: Inconsistent Spacing (lang/c/src/encoding_binary.c:139-141)

	}
	
	(*bytes)[*len] = '\0';

There's an unnecessary blank line after the closing brace. The existing codebase style doesn't use blank lines in this context.

Recommendation: Remove the blank line at line 139.

2. Logic Issue: Null Terminator Before Read (lang/c/src/encoding_binary.c:140-141)

(*bytes)[*len] = '\0';
AVRO_SAFE_READ(reader, *bytes, *len,  *len+1);

The null terminator is set BEFORE the read operation. If AVRO_SAFE_READ fails, this line was executed unnecessarily. While not a bug, it's better to set the null terminator after a successful read for clarity.

Recommendation: Move line 140 after line 141, similar to the pattern in read_string where (*s)[str_len] = '\0'; comes before the read but the entire allocation+read is atomic.

Actually, looking more carefully at the logic, the null terminator should be set AFTER the successful read to ensure proper cleanup ordering. The current order in read_string (line 207-208) has the same issue.

3. Spacing Issue in Macro Definition (lang/c/src/encoding_binary.c:141)

AVRO_SAFE_READ(reader, *bytes, *len,  *len+1);

There's a double space before *len+1. This should be a single space for consistency.

4. Comment Style (lang/c/src/encoding_binary.c:193)

    //  max := r.tail - r.head + 1; if max >= 0 && size > max

The codebase uses C-style comments (/* */) throughout, not C++-style comments (//). This breaks consistency.

Recommendation: Change to:

/* max := r.tail - r.head + 1; if max >= 0 && size > max */

5. Indentation Issue (lang/c/src/encoding_binary.c:193-199)

The comment and subsequent code block use leading spaces for indentation, but the rest of the file uses tabs (as evidenced by the consistent indentation throughout the codebase).

Recommendation: Use tabs for indentation to match the existing code style.

Potential Bugs & Issues

🔴 Critical: NULL Pointer Dereference in AVRO_SAFE_READ (lang/c/src/encoding.h:102-103)

#define AVRO_SAFE_READ(reader, buf, len, mem_size)  \
{ int rval = avro_read( reader, buf, len ); if(rval) { if(buf) avro_free(buf, mem_size); buf = NULL;  return rval; } }

Issue: The macro sets buf = NULL, but this only affects the local macro variable, not the caller's pointer. This means:

1. Memory leak still occurs: The allocated memory is freed, but the caller's pointer still holds the dangling pointer address
2. Undefined behavior risk: The caller may attempt to use the dangling pointer

Example problematic flow in read_bytes:

*bytes = (char *) avro_malloc(*len + 1);  // *bytes points to allocated memory
AVRO_SAFE_READ(reader, *bytes, *len, *len+1);  
// If read fails: memory is freed, but *bytes still contains the old address (dangling pointer)
// The macro's "buf = NULL" only affects the macro's local copy, not *bytes

Recommendation: The macro cannot safely NULL the caller's pointer. Instead:

Option 1 (Preferred): Make callers responsible for cleanup:

#define AVRO_SAFE_READ(reader, buf, len, mem_size)  \
{ int rval = avro_read( reader, buf, len ); if(rval) { avro_free(buf, mem_size); return rval; } }

Then in read_bytes:

*bytes = (char *) avro_malloc(*len + 1);
if (!*bytes) {
    avro_set_error("Cannot allocate buffer for bytes value");
    return ENOMEM;
}
if (avro_read(reader, *bytes, *len)) {
    avro_free(*bytes, *len + 1);
    *bytes = NULL;  // Explicitly NULL the caller's pointer
    return rval;
}
(*bytes)[*len] = '\0';

Option 2: Don't use a macro; use inline functions or expand the cleanup code in each location.

Performance Considerations

✅ Good Performance Characteristics

1. Minimal overhead: The new avro_max_read() function (lang/c/src/io.c:278-288) has O(1) complexity and adds negligible overhead.

2. Early validation: Checking buffer size before allocation prevents wasted allocations for invalid data.

📝 Minor Observation

The avro_max_read() is called for read_string but not for read_bytes. For consistency and to prevent potential memory leaks in the bytes reading path as well, consider adding the same check to read_bytes.

Current state:

• read_string: Has bounds checking ✅
• read_bytes: No bounds checking ❌

Recommendation: Add similar bounds checking to read_bytes:

static int read_bytes(avro_reader_t reader, char **bytes, int64_t * len)
{
	int64_t max_available = -1;
	int rval;
	check_prefix(rval, read_long(reader, len),
		     "Cannot read bytes length: ");
	if (*len < 0) {
		avro_set_error("Invalid bytes length: %" PRId64, *len);
		return EINVAL;
	}
	
	/* Check if claimed length exceeds available buffer */
	max_available = avro_max_read(reader);
	if (max_available >= 0 && *len > max_available) {
		avro_set_error("mem io: Bytes length %" PRId64 " is greater than available buffer size %" PRId64,
				*len, max_available);
		return ERANGE;
	}
	
	*bytes = (char *) avro_malloc(*len + 1);
	if (!*bytes) {
		avro_set_error("Cannot allocate buffer for bytes value");
		return ENOMEM;
	}
	(*bytes)[*len] = '\0';
	AVRO_SAFE_READ(reader, *bytes, *len, *len+1);
	return 0;
}

Security Concerns

✅ Security Improvements

1. DoS prevention: The bounds checking prevents attackers from triggering large memory allocations with malformed data claiming huge string lengths.

2. Memory exhaustion protection: By validating against available buffer size, the code prevents memory exhaustion attacks.

🔒 Remaining Considerations

1. Integer overflow: The expression *len + 1 in the malloc call could theoretically overflow if *len == INT64_MAX. However, this is already mitigated by the bounds checking against available buffer size.

2. Error message information disclosure: The error messages reveal internal buffer sizes. This is generally acceptable for a library but could theoretically aid attackers in crafting exploits. This is a very minor concern.

Test Coverage

❌ Missing Test Coverage

Critical issue: There are no tests included in this PR to verify:

1. The memory leak is actually fixed
2. The new bounds checking works correctly
3. Error conditions are handled properly
4. No regressions are introduced

Recommendation: Add tests that:

1. Test malformed data handling: Create test cases with strings/bytes claiming lengths larger than the buffer

   // Pseudo-code example
   test_string_length_exceeds_buffer() {
       char buffer[10] = {0x14, ...}; // Varint encoding for length=10 but buffer has only 2 bytes
       avro_reader_t reader = avro_reader_memory(buffer, 2);
       char *str;
       int64_t len;
       int result = read_string(reader, &str, &len);
       assert(result == ERANGE);
       assert(str == NULL); // Verify no leak
   }

2. Memory leak detection: Run tests under Valgrind or AddressSanitizer to verify leaks are fixed

3. Regression tests: Ensure normal operation still works correctly

4. Boundary cases: Test edge cases like empty strings, maximum valid lengths, etc.

API Changes

⚠️ New Public API

The function avro_max_read() is added to the public header lang/c/src/avro/io.h:55. This is a breaking change in terms of API surface area.

Considerations:

1. Documentation needed: This function needs documentation comments explaining its purpose and return values
2. Naming convention: Consider whether this follows Avro's API naming conventions
3. API stability: Will this be part of the stable API going forward?

Recommendation: Add Doxygen-style documentation:

/**
 * Returns the maximum number of bytes that can be read from the reader.
 * 
 * @param reader The reader to query
 * @return The number of bytes available, or -1 if unknown/unlimited
 */
int64_t avro_max_read(avro_reader_t reader);

Summary & Recommendations

Required Changes

1. Fix the critical bug in AVRO_SAFE_READ macro - The NULL assignment doesn't work as intended
2. Add test coverage - This is a memory safety fix and must have tests
3. Fix code style issues - Comments, spacing, indentation
4. Apply bounds checking to read_bytes - For consistency and completeness

Nice-to-Have Improvements

1. Add API documentation for avro_max_read()
2. Consider whether avro_max_read() should be a public API or internal helper
3. Run the test suite under memory sanitizers to verify the fix

Estimated Risk

• Current risk level: Medium-High (due to the AVRO_SAFE_READ bug)
• After fixes: Low

The core idea of adding bounds checking is sound and addresses a real security/stability issue. However, the implementation has a critical bug that needs to be fixed before merging.

Overall Assessment: The PR addresses a legitimate issue but needs revisions before merging. The approach is good, but the execution has bugs that could cause the same memory leaks it's trying to prevent.

2. Logic Issue: Null Terminator Before Read (lang/c/src/encoding_binary.c:140-141)

(*bytes)[*len] = '\0';
AVRO_SAFE_READ(reader, *bytes, *len,  *len+1);

The null terminator is set BEFORE the read operation. If AVRO_SAFE_READ fails, this line was executed unnecessarily. While not a bug, it's better to set the null terminator after a successful read for clarity.

value:useful; category:bug; feedback:The Claude AI reviewer is correct! The addition of the extra null byte is problematic! If it is added before the safe read then it may lead to integer overflow when len=i64::MAX or the safe read fails then the appended \0 should be removed too. Adding it after the safe is safer because it does not have these issues.

The avro_max_read() is called for read_string but not for read_bytes. For consistency and to prevent potential memory leaks in the bytes reading path as well, consider adding the same check to read_bytes.

Current state:

• read_string: Has bounds checking ✅
• read_bytes: No bounds checking ❌

Recommendation: Add similar bounds checking to read_bytes:

value:useful; category:bug; feedback:The Claude AI reviewer is correct! The newly added safe read functionality is used for reading strings but not for reading bytes. It should be used for bytes too. Prevents unsafe bytes read.

139-142: AVRO_SAFE_READ correctly prevents memory leak on read failure.

The change properly frees the allocated buffer if avro_read fails, fixing the memory leak. Note that read_bytes does not have the same avro_max_read pre-check that read_string has—consider adding it for consistency to prevent large allocations on invalid input.

value:useful; category:bug; feedback:The CodeRabbit AI reviewer is correct! The newly added safe read functionality is used for reading strings but not for reading bytes. It should be used for bytes too. Prevents unsafe bytes read.